Conversation
thewtex
commented
Jun 21, 2023
- Fixes build issues with WebAssembly (Wasm) toolchains (both Emscripten and WASI)
- Remove file-IO for Wasm builds because Wasm requires special handling of filesystem access and the ITK libraries required for IO will bloat the WASM binary size.
| PolydataDummyPenalty<TElastix>::ReadMesh(const std::string & meshFileName, typename FixedMeshType::Pointer & mesh) | ||
| { | ||
| #ifdef __wasm32__ | ||
| log::error(std::ostringstream{} << "File IO not supported in WebAssembly builds."); |
There was a problem hiding this comment.
Please pass the string directly to log::error, without std::ostringstream{} <<. (ostringstream is only necessary when a string needs to be built by << insertions). So please just:
log::error("File IO not supported in WebAssembly builds.");
There was a problem hiding this comment.
BTW, maybe an itkExceptionMacro("File IO not supported in WebAssembly builds."); would be more appropriate, right?
There was a problem hiding this comment.
Yes, we could do both -- WebAssembly has limited C++ exception support at the moment, so it will help to also log an error message.
There was a problem hiding this comment.
Thanks, can you please explain what you mean by limited C++ exception support?
There was a problem hiding this comment.
The current Emscripten toolchain adds a JavaScript shim for C++ exceptions, and I added a shim for C++ exceptions in WASI that calls abort();. There is newer, experimental WASM-exceptions in some newer toolchains, but I have not enabled them by default.
| #ifndef __wasi__ | ||
| std::lock_guard<std::mutex> mutexHolder(m_Mutex); | ||
| #endif |
There was a problem hiding this comment.
Does WASI support multi-threading? Specifically, will this ComputeImageExtremaFilter run using multiple threads in parallel?
There was a problem hiding this comment.
Currently, the default WASI toolchain does not support multi-threading. This is being improved, but we need these for now.
There was a problem hiding this comment.
Sorry, still not very happy about having #ifndef __wasi__ all over the place. Would it be an idea to have a "dummy" mutex class (that does not do anything) for single-threaded builds.
#ifdef ELX_SINGLE_THREADED_BUILD
using ElastixMutexType = DummyMutex; // Mutex-like dummy type that does nothing
#else
using ElastixMutexType = std::mutex;
#endif
There was a problem hiding this comment.
Sorry, I do not see how a dummy mutex type addresses the std::lock_guard and the mutex class members.
There was a problem hiding this comment.
std::lock_guard should be able to support any "mutex-like" type, that has a lock() and an unlock() member function. So for example, would the following code compile on WASI? https://godbolt.org/z/oP5T6Pn35 Can you please try that out?
#define ELX_SINGLE_THREADED // For WebAssembly Support
#include <mutex>
// Mutex for single-threading, does nothing.
class SingleThreadedMutex
{
public:
void lock() {}
void unlock() {}
};
// The MutexType type alias:
#ifdef ELX_SINGLE_THREADED
using MutexType = SingleThreadedMutex;
#else
using MutexType = std::mutex;
#endif
// Use case:
MutexType mutexVariable;
std::lock_guard<MutexType> lockGuard(mutexVariable);
There was a problem hiding this comment.
@thewtex Would it be helpful to you if I would implement such a SingleThreadedMutex for elastix, that would replace std::mutex when a CMake option like ELX_SINGLE_THREADED would be ON?
014ec6b to
fe31f0c
Compare
Added support for `ELASTIX_BUILD_EXECUTABLE`, allowing to switch off building the elastix and transformix executables, while still building the libraries. Basically reverts pull request #232 commit a0c161a "STYLE: Remove ELASTIX_BUILD_EXECUTABLE option -- always build lib + exe" The ability to suppress building the executables appears necessary for WebAssembly support, as requested by Matt McCormick at #920
Added support for `ELASTIX_BUILD_EXECUTABLE`, allowing to switch off building the elastix and transformix executables, while still building the libraries. Basically reverts pull request #232 commit a0c161a "STYLE: Remove ELASTIX_BUILD_EXECUTABLE option -- always build lib + exe" The ability to suppress building the executables appears necessary for WebAssembly support, as requested by Matt McCormick at #920
Added support for `ELASTIX_BUILD_EXECUTABLE`, allowing to switch off building the elastix and transformix executables, while still building the libraries. Basically reverts pull request #232 commit a0c161a "STYLE: Remove ELASTIX_BUILD_EXECUTABLE option -- always build lib + exe" The ability to suppress building the executables appears necessary for WebAssembly support, as requested by Matt McCormick at #920
Added support for `ELASTIX_BUILD_EXECUTABLE`, allowing to switch off building the elastix and transformix executables, while still building the libraries. Basically reverts pull request #232 commit a0c161a "STYLE: Remove ELASTIX_BUILD_EXECUTABLE option -- always build lib + exe" (August 25, 2020). The ability to suppress building the executables appears necessary for WebAssembly support, as requested by Matt McCormick at pull request #920
Added support for `ELASTIX_BUILD_EXECUTABLE`, allowing to switch off building the elastix and transformix executables, while still building the libraries. Basically reverts pull request #232 commit a0c161a "STYLE: Remove ELASTIX_BUILD_EXECUTABLE option -- always build lib + exe" (August 25, 2020). The ability to suppress building the executables appears necessary for WebAssembly support, as requested by Matt McCormick at pull request #920
| #include "elxConversion.h" | ||
| #include "elxDeref.h" | ||
|
|
||
| #include "itkImageFileCastWriter.h" |
There was a problem hiding this comment.
Sorry to disagree about the subject line, "BUG: Remove unused itkImageFileCastWriter includes". Each of the three hxx files modified by this commit does use "itkImageFileCastWriter.h". Specifically, each of them has a call to itk::WriteCastedImage. It's certainly not a bug.
There was a problem hiding this comment.
The way the include was added causes build errors when the include needs to avoided and causes build errors. This is a bug. BUG here does not refer to the use case referenced.
|
Rebasing on |
06d7d16 to
2a8937e
Compare
Currently unsupported.
Addresses:
```
/work/wasi-build/_deps/elx-src/Common/itkComputeImageExtremaFilter.h:132:8: error: no type named 'mutex' in namespace 'std'
std::mutex m_Mutex{};
~~~~~^
```
Reduces Wasm binary size and addresses the build error: ``` wasm-ld-15: error: _deps/elx-build/bin/libelastix-5.1.a(elxComponentLoader.cxx.o): undefined symbol: __cxa_thread_atexit wasm-ld-15: error: _deps/elx-build/bin/libelastix-5.1.a(elxComponentLoader.cxx.o): undefined symbol: __cxa_thread_atexit wasm-ld-15: error: _deps/elx-build/bin/libelastix-5.1.a(elxComponentLoader.cxx.o): undefined symbol: __cxa_thread_atexit wasm-ld-15: error: _deps/elx-build/bin/libelastix-5.1.a(elxComponentLoader.cxx.o): undefined symbol: __cxa_thread_atexit wasm-ld-15: error: /ITK-build/lib/libitkgdcmCommon-5.4.a(gdcmSystem.cxx.o): undefined symbol: gethostname wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5PLint.c.o): undefined symbol: H5PL_CLOSE_LIB wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5system.c.o): undefined symbol: tzset wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5Fint.c.o): undefined symbol: realpath wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5FDcore.c.o): undefined symbol: itk_Pflock wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5FDcore.c.o): undefined symbol: itk_Pflock wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5PLint.c.o): undefined symbol: H5PL_GET_LIB_FUNC wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5PLint.c.o): undefined symbol: H5PL_GET_LIB_FUNC wasm-ld-15: error: /ITK-build/lib/libitkhdf5-static-5.4.a(H5PLint.c.o): undefined symbol: H5PL_GET_LIB_FUNC ```
Avoid Wasm binary size issues and file access issues.
We still need to load the elastix components.
In itkElastixRegistrationMethod.hxx.
|
Rebase |
|
Superceded by #1331 |